Stop SSE filter from leaking tools/list on undecodable lines#5304
Stop SSE filter from leaking tools/list on undecodable lines#5304saivedant169 wants to merge 4 commits into
Conversation
processSSEResponse used to write the entire raw upstream payload and return whenever a single SSE data line failed jsonrpc2.DecodeMessage or decoded to a non-Response message (e.g. a notifications/* frame). On a tools/list reply that meant every subsequent data line, including the real Response, reached the client unfiltered, silently bypassing the cedar authorization filter and producing the superfluous WriteHeader warning at response_filter.go:191. Treat undecodable or non-Response data lines as pass-through for that line only: fall through to the existing line writer instead of dumping rawResponse. The explicit WriteHeader calls go away with them, which also removes the double-header warning that surfaced the bug. Skipped filtering on tools/list now emits a WARN so future bypasses are visible in audit logs. Adds a table-driven regression test covering both branches (decode error and non-Response). It fails on the old code with the unfiltered admin_tool entry reaching the recorder. Closes stacklok#5257
saivedant169
requested review from
ChrisJBurns,
JAORMX,
jhrozek and
yrobla
as code owners
|
This PR looks like it's targetting issue #5292 ? |
|
cc @amirejaz |
|
Not quite — #5304 closes #5257, which is an authorization bypass in the Cedar SSE response filter ( Our issue #5292 is a separate concern in a different layer: per-event JSON-RPC frame validation in the transparent proxy ( The two are complementary — #5304 fixes a more serious auth bypass, #5292 is about frame validation in the non-auth transparent proxy path. |
yrobla
left a comment
yrobla
left a comment
There was a problem hiding this comment.
The direction is correct and the core fix is sound — removing the whole-payload dump and letting the existing if !written branch handle per-line passthrough is exactly right. Two warnings below about logging coverage and test coverage.
| // Pass this line through unfiltered. Earlier revisions wrote | ||
| // rawResponse and returned here, which leaked every subsequent | ||
| // data line on the stream past the filter (issue #5257). | ||
| if rfw.method == string(mcp.MethodToolsList) { |
There was a problem hiding this comment.
Warning: WARN logs are only emitted for tools/list, but the bypass applies to all four filtered methods.
Both log conditions — err != nil branch here, and the else if for non-*jsonrpc2.Response in the default: branch below — are gated on rfw.method == string(mcp.MethodToolsList). However, requiresResponseFiltering covers four methods: tools/list, prompts/list, resources/list, and find_tool. An SSE backend serving prompts/list or resources/list with interleaved MCP notifications would silently hit the same passthrough path — no log line at all.
The security fix itself is correct for all methods (the written flag controls passthrough regardless of method), but the observability goal from the issue — "future bypasses surface in audit logs rather than going silent" — is only fulfilled for tools/list.
Consider replacing both method checks with requiresResponseFiltering(rfw.method), which already encodes the right set.
| // an MCP notification) or an undecodable data line with a real tools/list | ||
| // response, the filter previously wrote the entire raw upstream payload and | ||
| // returned, leaking the unfiltered tools/list past Cedar. It must instead pass | ||
| // only the offending line through and continue filtering the rest of the |
There was a problem hiding this comment.
Warning: Regression test only covers MethodToolsList; the same bypass applies to prompts/list and resources/list over SSE.
The new test hardcodes string(mcp.MethodToolsList). The same code path in processSSEResponse runs for any method where requiresResponseFiltering is true — including MethodPromptsList and MethodResourcesList. A notification-interleaved SSE response on those paths is not currently exercised.
A brief additional sub-test for MethodPromptsList or MethodResourcesList with an interleaved notification would confirm parity and guard against a future regression on those paths.
Address review feedback on stacklok#5304: The WARN log paths in processSSEResponse were gated on the tools/list method, but the underlying bypass applies to every method routed through requiresResponseFiltering (tools/list, prompts/list, resources/list, find_tool). Drop the gating so the WARN fires for any of them. The regression test only exercised tools/list. Restructure it into a method-by-preceding-line table so the same notification and undecodable line cases run against prompts/list and resources/list as well. Verified the WARN now logs for all three methods and the unauthorized entry never reaches the recorder.
|
Both points addressed in 3984262. The WARN logs no longer gate on tools/list, so any method that hits processSSEResponse via requiresResponseFiltering (tools/list, prompts/list, resources/list, find_tool) now WARNs when a data line is undecodable or non-Response. The regression test is now a method-by-preceding-line table over all three list methods. Each method has its own cedar policy and a one-authorized + one-unauthorized fixture; both the notification and undecodable-line cases run against tools/list, prompts/list, and resources/list. Confirmed the unauthorized entry never reaches the recorder for any of the six combinations. @yrobla ready for another look when you have a moment. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5304 +/- ##
=======================================
Coverage 68.40% 68.40%
=======================================
Files 624 624
Lines 63442 63442
=======================================
+ Hits 43397 43400 +3
+ Misses 16807 16805 -2
+ Partials 3238 3237 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
size/S
4 participants
Summary
ResponseFilteringWriter.processSSEResponseused to abort filtering of the entire SSE stream whenever a singledata:line failedjsonrpc2.DecodeMessageor decoded to a non-*jsonrpc2.Responsemessage. The fallback wrote the entire buffered upstream payload and returned, so any subsequentdata:line containing the realtools/listreply reached the client unfiltered, bypassing the cedar authorization filter. The fallback also re-calledWriteHeaderafter the reverse proxy's firstFlush()had already emitted headers, producing thehttp: superfluous response.WriteHeader call from … response_filter.go:191warning that surfaced the bug.This PR:
Responsedata:line as pass-through for that line only: fall through to the existing line writer (the loop'sif !writtenbranch) rather than dumpingrawResponseand returning.WriteHeader(rfw.statusCode)calls in the fallback branches, which removes the double-header warning at line 191.tools/listreply contains adata:line that bypasses the filter, so future bypasses are visible in audit logs instead of silent.Notifications interleaved on a response stream are explicitly allowed by the MCP spec, so this case can be hit by fully spec-compliant upstreams (
notifications/message, progress updates, etc.) as well as by upstreams fronted by an SSE bridge (the npmmcp-proxycase from the issue reproduction).Closes #5257
Type of change
Test plan
task test)task lint-fix)Added
TestResponseFilteringWriter_SSE_PerLineFallthroughas a table-driven regression test covering both branches (decode error and non-Responsedecode). It builds an SSE stream that interleaves a notification (or an undecodable garbage line) with a realtools/listresponse, then asserts:tools/listresponse is filtered to only authorized tools."admin_tool"(the unauthorized tool from the unfiltered upstream payload) does not appear anywhere in the output.The test fails on the prior code with
admin_toolreaching the recorder, and passes on the patched code.Verified on
pkg/authz/...with-race:go test -ldflags=-extldflags=-Wl,-w -race ./pkg/authz/...passes (including the existing JSON-path tests and the new SSE test).golangci-lint run ./pkg/authz/...reports 0 issues.Does this introduce a user-facing change?
Yes. On SSE upstreams (
Content-Type: text/event-stream),tools/list,prompts/list, andresources/listresponses now respect the configured cedar authorization filter even when the upstream interleaves notifications or sends an undecodabledata:line. Previously such streams returned the unfiltered tool/prompt/resource catalog to the caller. Filtered behavior is what operators already get onapplication/jsonupstreams; this brings SSE in line.Special notes for reviewers
The change is intentionally minimal. I considered also auditing
processJSONResponsefor the same shape (return rawResponse on per-line failure) but it processes a single message, not a stream, so the same code shape there is already correct. Happy to extend if you want a defensive log added on that side too.